Conditionally add monitoring structure to top-level of output in API #11106

cachedout · 2019-08-31T19:22:25Z

Adds a new setting to Logstash called xpack.monitoring.cluster_uuid. If the setting is discovered to be set by the user, it is added to the top-level of API responses.

@ycombinator I have included instructions below for you to have a look and see if it agrees with what you were expecting:

Testing

Add xpack.monitoring.cluster_uuid: OVERRIDE to logstash.yml configuration file.
Start logstash
Issue an HTTP GET to an endpoint such as http://localhost:9600/_node/stats
Verify that the response contains the following key-value pair at the top-level of the dictionary: "cluster_uuid: OVERRIDE"
Stop Logstash
Modify logstash.yml and comment out the setting in step 1
Restart Logstash and re-issue the query in step 3. Observe that the top-level cluster_uuid does not exist.

Questions

From reading the issue it seems like the only requested functionality is to add the key to the API response and not to modify the return in any other way. Is that understanding correct?

ycombinator · 2019-09-06T12:56:20Z

I ran this PR locally and it works as expected. So I think this is definitely going in the right direction!

A couple of thoughts, mostly around where the new field should go in the API response body:

Perhaps it's too odd to just have cluster_uuid as a top-level key in the API response? After all the concept of a cluster UUID isn't something that's core to Logstash itself. Some alternatives could be naming the field xpack.monitoring.cluster_uuid (so same as the setting), or even creating a new settings section so the field name becomes settings.xpack.monitoring.cluster_uuid?
Rather than adding this top-level cluster_uuid field to multiple API responses, perhaps we should have a new API endpoint, something like GET _node/settings that exposes certain whitelisted settings from the Logstash configuration? Or maybe this could just be part of the current GET _node API (but not other APIs)?

[EDIT] I don't have strong preferences about these options. I think it might be good to get someone from Logstash core to weigh in on this whenever the PR is ready for review.

From reading the PR it seems like the only requested functionality is to add the key to the API response and not to modify the return in any other way. Is that understanding correct?

This is correct.

If the key is present, Metricbeat will consume it and use it's value as the value of the cluster_uuid field in .monitoring-logstash-* documents. If it's not present, Metricbeat will do what it's doing today — try and determine the value from any ES output vertices (if the LS pipeline is using the ES output), or failing that, just not set a cluster_uuid field in .monitoring-logstash-* documents.

cachedout · 2019-09-06T13:49:13Z

@jsvd Any thoughts on the comments from @ycombinator above?

I don't think I have particularly strong opinions, either. I could really live with any of the excellent options outlined by @ycombinator. If I had to choose, I'd maybe pick a settings key in the top-level of the return structure where we could put this type of data but I can live with any of the options.

cachedout · 2019-09-10T14:16:42Z

Bump @jsvd . Thanks!

jsvd · 2019-09-10T16:21:47Z

if we're only planning to add this cluster_uuid then I'd probably put it alongside the node uuid and other more static information inside the node info (/_node) api, something like:

% curl -XGET localhost:9600/_node/ | jq
{
  "host": "overcraft.lan",
  "version": "7.4.0",
  "http_address": "127.0.0.1:9600",
  "id": "26e7b7cf-94f0-443e-b2b4-d149eeb07fae",
  "name": "overcraft.lan",
  "ephemeral_id": "9d823dfd-7ab9-4731-9638-0191144a01c2",
  "status": "green",
  "snapshot": false,
  "monitoring": {
    "cluster_uuid": "AAAAAA",
  },
  "pipeline": {
    "workers": 8,
    "batch_size": 125,
    "batch_delay": 50
  },
  "pipelines": {
  ....

I am adding the "monitoring" namespace as I'm concerned about naming here, as this can mislead folks into thinking their logstashes are clustered in some way.

Not necessary for this iteration, but since we're adding this cluster uuid return through the api, could we also report a few other details like host and user (not password)? having these would make it clearer that the cluster uuid refers to the ES cluster for monitoring:

  "monitoring": {
    "cluster_uuid": "AAAAAA",
    "hosts": [ "es1:9200" ],
    "user": [ "logstash_monitoring"]
  },

cachedout · 2019-09-11T15:44:19Z

@ycombinator and @jsvd I've pushed up some changes to incorporate the feedback. Let me know what you think. Thanks!

ycombinator · 2019-09-17T02:43:02Z

I just pulled down this PR and ran the following Logstash pipeline with it:

input { stdin {} }

A couple of observations:

When using the default logstash.yml (i.e. with no settings changed or added), I see an empty monitoring: {} property in the GET / API response. Is this what we want? Or do we want to completely omit the monitoring property if no child properties (e.g. monitoring.cluster_uuid) are set?

When setting xpack.monitoring.cluster_uuid: foobar in logstash.yml (but making no other changes/additions to it), the GET / API returns the following error response:

{
  "status": 500,
  "request_method": "GET",
  "path_info": "/",
  "query_string": "",
  "http_version": "HTTP/1.1",
  "http_accept": "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3",
  "error": "Unexpected Internal Error",
  "class": "TypeError",
  "message": "no implicit conversion of String into Hash",
  "backtrace": [
    "org/jruby/RubyHash.java:1899:in `merge!'",
    "org/jruby/RubyHash.java:1935:in `merge'",
    "/Users/shaunak/development/github/logstash/logstash-core/lib/logstash/api/commands/default_metadata.rb:10:in `all'",
    "/Users/shaunak/development/github/logstash/logstash-core/lib/logstash/api/app_helpers.rb:52:in `default_metadata'",
    "/Users/shaunak/development/github/logstash/logstash-core/lib/logstash/api/app_helpers.rb:28:in `respond_with'",
    "/Users/shaunak/development/github/logstash/logstash-core/lib/logstash/api/modules/root.rb:8:in `block in GET /'",
    "org/jruby/RubyMethod.java:132:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1611:in `block in compile!'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:975:in `block in route!'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:994:in `route_eval'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:975:in `block in route!'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1015:in `block in process_route'",
    "org/jruby/RubyKernel.java:1193:in `catch'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1013:in `process_route'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:973:in `block in route!'",
    "org/jruby/RubyArray.java:1792:in `each'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:972:in `route!'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1085:in `block in dispatch!'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1067:in `block in invoke'",
    "org/jruby/RubyKernel.java:1193:in `catch'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1067:in `invoke'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1082:in `dispatch!'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:907:in `block in call!'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1067:in `block in invoke'",
    "org/jruby/RubyKernel.java:1193:in `catch'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:1067:in `invoke'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:907:in `call!'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:895:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-protection-1.5.5/lib/rack/protection/xss_header.rb:18:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-protection-1.5.5/lib/rack/protection/path_traversal.rb:16:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-protection-1.5.5/lib/rack/protection/json_csrf.rb:18:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-protection-1.5.5/lib/rack/protection/base.rb:49:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-protection-1.5.5/lib/rack/protection/base.rb:49:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-protection-1.5.5/lib/rack/protection/frame_options.rb:31:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-1.6.11/lib/rack/nulllogger.rb:9:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-1.6.11/lib/rack/head.rb:13:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:182:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/sinatra-1.4.8/lib/sinatra/base.rb:2013:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-1.6.11/lib/rack/urlmap.rb:66:in `block in call'",
    "org/jruby/RubyArray.java:1792:in `each'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-1.6.11/lib/rack/urlmap.rb:50:in `call'",
    "/Users/shaunak/development/github/logstash/logstash-core/lib/logstash/api/rack_app.rb:57:in `call'",
    "/Users/shaunak/development/github/logstash/logstash-core/lib/logstash/api/rack_app.rb:31:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/rack-1.6.11/lib/rack/builder.rb:153:in `call'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/puma-2.16.0-java/lib/puma/server.rb:557:in `handle_request'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/puma-2.16.0-java/lib/puma/server.rb:404:in `process_client'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/puma-2.16.0-java/lib/puma/server.rb:270:in `block in run'",
    "/Users/shaunak/development/github/logstash/vendor/bundle/jruby/2.5.0/gems/puma-2.16.0-java/lib/puma/thread_pool.rb:106:in `block in spawn_thread'"
  ]
}

The same error is also shown in the Logstash server logs.

cachedout · 2019-09-17T11:39:51Z

@ycombinator I've addressed both issues that you reported. (Thanks for finding it!)

However, I'd like to add more testing around this before taking it out of Draft status. I should be able to get to that by the end of the week.

ycombinator · 2019-09-17T15:13:23Z

Thanks @cachedout. I'll wait to do the next round of review until this PR is in "Ready for Review" state, since we're expecting more changes/additions to this PR.

cachedout · 2019-09-24T14:28:05Z

@jsvd I could use some help on this one. I've tried a couple different approaches to temporarily setting a settings value (https://github.com/elastic/logstash/pull/11106/files#diff-430ede93a6bae9fb57425ef58be7186fR24) and no matter what, tests fail on the setting not being registered, even when I explicitly register it after each test. Do you know what's going on here?

cachedout · 2019-10-07T09:19:50Z

Hi again @jsvd. Any chance we could carve out a little time to try to figure out what's going on with these tests? I've hit a dead end.

cachedout · 2019-10-18T12:33:56Z

Hi @jsvd. Just checking if you might have some time in the coming days to assist with this. Thanks!

cachedout · 2019-12-02T13:27:49Z

Hi again @jsvd. Is there anybody on the LS team who might be able to spare a little time to help out here? Thanks. :)

cachedout · 2020-01-16T13:56:11Z

@jsvd I suspect the @elastic/stack-monitoring team is still looking for this to get in at some point. I'm happy to take some time to help get this finished but will need some help from the LS folks since I'm at a loss to explain this test failure, I'm afraid.

cachedout · 2020-01-21T12:05:31Z

Pinged Logstash team in Slack and awaiting a response.

andsel · 2020-01-21T13:50:56Z

Jenkins test this please

roaksoax · 2020-01-21T14:23:55Z

@cachedout @andsel will be looking into this PR, we will target this for 7.7. Let me know if there's anything else you need?

andsel · 2020-01-22T13:36:05Z

logstash-core/spec/logstash/api/commands/default_metadata_spec.rb

@@ -12,9 +12,27 @@

  let(:report_class) { described_class }

+  after :each do
+    LogStash::SETTINGS.register(LogStash::Setting::Boolean.new("xpack.monitoring.enabled", false)) unless LogStash::SETTINGS.registered?("xpack.monitoring.enabled")
+    LogStash::SETTINGS.reset


LogStash::SETTINGS is a singleton instance in the LogStash module, the reset operation nullify all the values in the collection, perhaps could be best to have a logic of save-before test restore-after for only the settings used by this test

andsel · 2020-01-23T08:24:15Z

Jenkins test this

andsel · 2020-01-23T10:52:15Z

Fixed the build and tested locally. So for me is ready for review

ycombinator · 2020-01-23T12:09:39Z

In terms of the HTTP API changes, this PR works as expected. However, I'm noticing another issue.

Let's say I have the following configuration in logstash.yml:

xpack.monitoring.enabled: true
xpack.monitoring.cluster_uuid: foobar

And let's say I'm running this pipeline:

input { stdin {} }

Because xpack.monitoring.enabled is set to true, Logstash will start up it's internal monitoring pipeline (monitoring-logstash) as well, and try to send monitoring data to the POST localhost:9200/_monitoring/bulk API.

However, the use case for this PR is for Metricbeat to be able to externally monitor a Logstash node by calling various Logstash HTTP APIs. So, in such a scenario, we wouldn't want Logstash itself also sending data to the monitoring Elasticsearch cluster.

Would it be possible to expose the value from the xpack.monitoring.cluster_uuid setting in the HTTP API responses without also requiring xpack.monitoring.enabled to be set to true in the configuration? FWIW, this is how Beats is implementing the analogous setting and exposing it in HTTP APIs.

ycombinator · 2020-01-23T12:18:34Z

Also, we should probably update this doc to mention this new setting, either as part of this PR itself or a follow up PR?

In the Beats docs, we document this setting like so:

monitoring:
  cluster_uuid: PRODUCTION_ES_CLUSTER_UUID

This setting identifies the Elasticsearch cluster under which the monitoring data for this Heartbeat instance will appear in the Stack Monitoring UI. To get a cluster’s cluster_uuid, call the GET / API against that cluster.

Of course, in Logstash the setting is under the xpack.monitoring namespace (as opposed to the monitoring namespace in Beats).

andsel · 2020-01-23T13:57:03Z

I agree with you @ycombinator it's not correct to enable xpack.monitoring (that then pushes monitoring data on its own) to expose the cluster_uuid. What I propose is to:

rename xpack.monitoring.cluster_uuid to monitoring.cluster_uuid
expose monitoring.cluster_uuid independently of X-Pack monitoring

ycombinator · 2020-01-23T15:05:40Z

I like @andsel's proposal. It will make the setting name for the cluster UUID, monitoring.cluster_uuid, consistent across Logstash and Beats which is nice for users.

andsel · 2020-01-24T11:30:44Z

@ycombinator I've integrated the your suggested changes. Updated the doc with ~~PR #11535~~ PR #11538

ycombinator

Functionally LGTM!

…e stats HTTP API

cachedout · 2020-01-25T13:37:32Z

@andsel Feel free to merge this when you think it's ready. :)

elasticsearch-bot · 2020-01-27T08:12:42Z

Andrea Selva merged this into the following branches!

Branch	Commits
master	`020e87e`
7.x	`d2a0946`

…e stats HTTP API Fixes #11106

chrisronline · 2020-02-07T15:36:41Z

I think PR is causing parity test failures: https://internal-ci.elastic.co/job/elastic+estf-monitoring-snapshots+master+multijob-logstash/78/console

It looks like the monitoring documents collected by Metricbeat contain (via the _node/stats api endpoint) an additional nested field within logstash_stats.logstash called monitoring and it contains a single key: cluster_uuid. This additional nested field does not show up in the monitoring documents collected internally.

ycombinator · 2020-02-07T15:38:41Z

Thanks @chrisronline. Given what you've described, I think the fix for this needs to go into the Metricbeat logstash module code. I'll put up a PR shortly.

ycombinator · 2020-02-07T17:02:26Z

@chrisronline PR is up: elastic/beats#16198. Could you functionally review it when you get a chance, please? Thanks!

cachedout changed the title ~~Conditionally add cluster_uuid to top-level of output in API~~ Conditionally add monitoring structure to top-level of output in API Sep 11, 2019

cachedout force-pushed the issue_11066 branch from ae08581 to ab8efdb Compare September 17, 2019 11:29

jsvd mentioned this pull request Dec 3, 2019

[DRAFT] Rebase of 11066 #11383

Closed

roaksoax assigned andsel Jan 21, 2020

roaksoax requested a review from andsel January 21, 2020 14:22

roaksoax added this to the v7.7.0 milestone Jan 21, 2020

andsel force-pushed the issue_11066 branch from 45b5e56 to d319405 Compare January 21, 2020 16:29

andsel reviewed Jan 22, 2020

View reviewed changes

ycombinator mentioned this pull request Jan 23, 2020

Metricbeat logstash module should honor monitoring.cluster_uuid field in Logstash API responses elastic/beats#15772

Closed

ycombinator mentioned this pull request Jan 23, 2020

Metricbeat logstash module: accept override cluster UUID from Logstash elastic/beats#15795

Merged

5 tasks

ycombinator approved these changes Jan 24, 2020

View reviewed changes

andsel force-pushed the issue_11066 branch from 6af7a6c to 7825473 Compare January 24, 2020 16:41

Add cluster_uuid setting to default config file, displaying it in Nod…

ec0c6e1

…e stats HTTP API

andsel force-pushed the issue_11066 branch from 7825473 to ec0c6e1 Compare January 24, 2020 16:44

elasticsearch-bot pushed a commit that referenced this pull request Jan 27, 2020

Add cluster_uuid setting to default config file, displaying it in Nod…

d2a0946

…e stats HTTP API Fixes #11106

elasticsearch-bot closed this in 020e87e Jan 27, 2020

andsel added v7.7.0 v8.0.0 labels Jan 27, 2020

andsel mentioned this pull request Jan 27, 2020

Add ability to override cluster_uuid to be used in monitoring data #11066

Closed

ycombinator mentioned this pull request Feb 7, 2020

Omit monitoring object from logstash_stats.logstash object elastic/beats#16198

Merged

1 task

ycombinator mentioned this pull request Feb 12, 2020

Adjustments to send monitoring data directly to ES #11541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditionally add monitoring structure to top-level of output in API #11106

Conditionally add monitoring structure to top-level of output in API #11106

cachedout commented Aug 31, 2019 •

edited

Loading

ycombinator commented Sep 6, 2019 •

edited

Loading

cachedout commented Sep 6, 2019

cachedout commented Sep 10, 2019

jsvd commented Sep 10, 2019

cachedout commented Sep 11, 2019

ycombinator commented Sep 17, 2019 •

edited

Loading

cachedout commented Sep 17, 2019

ycombinator commented Sep 17, 2019

cachedout commented Sep 24, 2019

cachedout commented Oct 7, 2019

cachedout commented Oct 18, 2019

cachedout commented Dec 2, 2019

cachedout commented Jan 16, 2020

cachedout commented Jan 21, 2020 •

edited

Loading

andsel commented Jan 21, 2020

roaksoax commented Jan 21, 2020

andsel Jan 22, 2020

andsel commented Jan 23, 2020

andsel commented Jan 23, 2020

ycombinator commented Jan 23, 2020 •

edited

Loading

ycombinator commented Jan 23, 2020 •

edited

Loading

andsel commented Jan 23, 2020 •

edited

Loading

ycombinator commented Jan 23, 2020

andsel commented Jan 24, 2020 •

edited

Loading

ycombinator left a comment

cachedout commented Jan 25, 2020

elasticsearch-bot commented Jan 27, 2020

chrisronline commented Feb 7, 2020

ycombinator commented Feb 7, 2020

ycombinator commented Feb 7, 2020

Conditionally add monitoring structure to top-level of output in API #11106

Conditionally add monitoring structure to top-level of output in API #11106

Conversation

cachedout commented Aug 31, 2019 • edited Loading

Testing

Questions

ycombinator commented Sep 6, 2019 • edited Loading

cachedout commented Sep 6, 2019

cachedout commented Sep 10, 2019

jsvd commented Sep 10, 2019

cachedout commented Sep 11, 2019

ycombinator commented Sep 17, 2019 • edited Loading

cachedout commented Sep 17, 2019

ycombinator commented Sep 17, 2019

cachedout commented Sep 24, 2019

cachedout commented Oct 7, 2019

cachedout commented Oct 18, 2019

cachedout commented Dec 2, 2019

cachedout commented Jan 16, 2020

cachedout commented Jan 21, 2020 • edited Loading

andsel commented Jan 21, 2020

roaksoax commented Jan 21, 2020

andsel Jan 22, 2020

Choose a reason for hiding this comment

andsel commented Jan 23, 2020

andsel commented Jan 23, 2020

ycombinator commented Jan 23, 2020 • edited Loading

ycombinator commented Jan 23, 2020 • edited Loading

andsel commented Jan 23, 2020 • edited Loading

ycombinator commented Jan 23, 2020

andsel commented Jan 24, 2020 • edited Loading

ycombinator left a comment

Choose a reason for hiding this comment

cachedout commented Jan 25, 2020

elasticsearch-bot commented Jan 27, 2020

chrisronline commented Feb 7, 2020

ycombinator commented Feb 7, 2020

ycombinator commented Feb 7, 2020

cachedout commented Aug 31, 2019 •

edited

Loading

ycombinator commented Sep 6, 2019 •

edited

Loading

ycombinator commented Sep 17, 2019 •

edited

Loading

cachedout commented Jan 21, 2020 •

edited

Loading

ycombinator commented Jan 23, 2020 •

edited

Loading

ycombinator commented Jan 23, 2020 •

edited

Loading

andsel commented Jan 23, 2020 •

edited

Loading

andsel commented Jan 24, 2020 •

edited

Loading